fix(NODE-3454): projection types are too narrow by nbbeeken · Pull Request #2924 · mongodb/node-mongodb-native

nbbeeken · 2021-07-30T17:05:23Z

With the introduction of aggregation operations in 4.4 in projection documents typing projections is very complex. I propose here we remove projection type constrains altogether to permit the many operators permitted by the server.

dariakp

It looks like there are two unused vars failing the build:

[2021/07/30 20:43:13.893] /data/mci/935b7979091d2b96139cddbecd5ed5ae/src/src/mongo_types.ts
[2021/07/30 20:43:13.893] 173:24 warning 'TSchema' is defined but never used @typescript-eslint/no-unused-vars
[2021/07/30 20:43:13.893] /data/mci/935b7979091d2b96139cddbecd5ed5ae/src/src/operations/find.ts
[2021/07/30 20:43:13.893] 21:30 warning 'TSchema' is defined but never used @typescript-eslint/no-unused-vars

dariakp · 2021-08-02T19:33:28Z

test/types/community/cursor.test-d.ts

+    .find({ _id: { $in: [] } })
+    .sort({ _id: -1 })
+    .limit(3)
+    .project<PublicMeme>(publicMemeProjection) // <== location of TS2345 error


what's TS2345?

Copy pasta-ed from the ticket its the type error that was occuring with the projection constraints, I put in the actual message for clarity.

I think including the ticket number would make sense, to provide context in the future about which TS2345 error this is referring to.

dariakp · 2021-08-02T19:36:37Z

test/types/community/cursor.test-d.ts

  await typedCollection.find().project({ notExistingField: 1 }).toArray()
 );
-expectNotType<TypedDoc[]>(await typedCollection.find().project({ notExistingField: 1 }).toArray());
+expectType<TypedDoc[]>(await typedCollection.find().project({ notExistingField: 1 }).toArray());


I thought this was supposed to yield Document[], which shouldn't match TypedDoc[]?

Corrected the test, I think this is correct, unless we want to change the return to always be generic?

This is part of the larger issue that we discovered, we'll tackle it in NODE-3468

dariakp · 2021-08-02T19:39:14Z

test/types/community/cursor.test-d.ts

+  likes: number;
+  someRandomProp: boolean; // Projection makes no enforcement on anything
+  // the convenience parameter project<X>() allows you to define a return type,
+  // otherwise projections returns a generic Document


I don't see a test to show an expectType<Document[]> here

PaulEndri · 2021-08-03T20:36:14Z

src/operations/find.ts

  sort?: Sort;
  /** The fields to return in the query. Object of fields to either include or exclude (one of, not both), `{'a':1, 'b': 1}` **or** `{'a': 0, 'b': 0}` */
-  projection?: Projection<TSchema>;
+  projection?: Document;


Wouldn't setting this to TSchema make the default functionality the same but still allow a more granular level of control without breaking things? And remove the need for the eslint disable

You mean setting projection?: TSchema? I think that would cause issue with the typical projection of inclusion / exclusion like { _id: 0, name: 1 }. where name: string in the TSchema

emadum · 2021-08-04T18:42:24Z

src/mongo_types.ts

+/**
+ * @public
+ * Projection is flexible to permit the wide array of aggregation operators
+ * @deprecated since v4.1.0: Since projections support all of aggregation operations we have no plans to narrow this type further


Suggested change

* @deprecated since v4.1.0: Since projections support all of aggregation operations we have no plans to narrow this type further

* @deprecated since v4.1.0: Since projections support all aggregation operations we have no plans to narrow this type further

emadum · 2021-08-04T18:42:50Z

src/mongo_types.ts

-  Partial<Record<string, ProjectionOperators | 0 | 1 | boolean>>;
+/**
+ * @public
+ * @deprecated since v4.1.0: Since projections support all of aggregation operations we have no plans to narrow this type further


Suggested change

* @deprecated since v4.1.0: Since projections support all of aggregation operations we have no plans to narrow this type further

* @deprecated since v4.1.0: Since projections support all aggregation operations we have no plans to narrow this type further

emadum · 2021-08-04T18:47:15Z

test/types/community/cursor.test-d.ts

+    .find({ _id: { $in: [] } })
+    .sort({ _id: -1 })
+    .limit(3)
+    .project<PublicMeme>(publicMemeProjection) // <== location of TS2345 error


I think including the ticket number would make sense, to provide context in the future about which TS2345 error this is referring to.

emadum

Few small requests but otherwise LGTM 👍

dariakp added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Aug 2, 2021

dariakp self-assigned this Aug 2, 2021

dariakp requested changes Aug 2, 2021

View reviewed changes

dariakp reviewed Aug 2, 2021

View reviewed changes

PaulEndri reviewed Aug 3, 2021

View reviewed changes

nbbeeken added 7 commits August 4, 2021 11:13

fix(NODE-3454): projection types are too narrow

b57859c

revert breakages

6810be3

fix: type import

fa9c234

fix: lint and tests

8bf04f4

fix: lint

d02193f

fix: deprecate the projection type

8c5f29f

fix: cursor returns generic doc

d8b55b4

nbbeeken force-pushed the NODE-3454/projection-types branch from b84b782 to d8b55b4 Compare August 4, 2021 15:14

fix: don't use overrrides for project

0e1ca61

nbbeeken requested a review from dariakp August 4, 2021 17:58

dariakp approved these changes Aug 4, 2021

View reviewed changes

dariakp added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Aug 4, 2021

dariakp requested review from durran and emadum August 4, 2021 18:23

emadum reviewed Aug 4, 2021

View reviewed changes

emadum suggested changes Aug 4, 2021

View reviewed changes

fix: comments

48aa991

nbbeeken requested a review from emadum August 4, 2021 18:51

emadum approved these changes Aug 4, 2021

View reviewed changes

dariakp merged commit 48d6da9 into 4.1 Aug 4, 2021

dariakp deleted the NODE-3454/projection-types branch August 4, 2021 19:58

ljhaywar pushed a commit that referenced this pull request Nov 9, 2021

fix(NODE-3454): projection types are too narrow (#2924)

95eb469

	* @deprecated since v4.1.0: Since projections support all of aggregation operations we have no plans to narrow this type further
	* @deprecated since v4.1.0: Since projections support all aggregation operations we have no plans to narrow this type further

Conversation

nbbeeken commented Jul 30, 2021

Uh oh!

dariakp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PaulEndri Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emadum left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PaulEndri Aug 3, 2021 •

edited

Loading